Conversation
TCBot Test Analysis
Possible Blockers (0)No blockers found. New Tests (18)
|
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves in-memory page eviction to support large rows and avoid lock-ordering deadlocks.
Changes:
- Adds size-aware eviction for single and batch inserts.
- Introduces non-blocking eviction locking and progress guards.
- Expands eviction regression tests for both LRU modes.
File summaries
| File | Description |
|---|---|
| modules/core/src/test/java/org/apache/ignite/testsuites/IgniteCacheEvictionSelfTestSuite.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/RandomLruPageEvictionWithExpiryPolicyTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/RandomLruPageEvictionSizeAwareTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/RandomLruPageEvictionConcurrentWritesTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/Random2LruPageEvictionWithExpiryPolicyTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/Random2LruPageEvictionSizeAwareTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/Random2LruPageEvictionConcurrentWritesTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionWithExpiryPolicyAbstractTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionSizeAwareAbstractTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionPutLargeObjectsAbstractTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionMetricTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionGuardOomTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionConcurrentWritesAbstractTest.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/RowStore.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/IgniteCacheDatabaseSharedManager.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/evict/PageAbstractEvictionTracker.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheMapEntry.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheEntryEx.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/configuration/DataRegionConfiguration.java | Updated as part of this pull request. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 11
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9ffb5d4 to
956bfc5
Compare
956bfc5 to
4d6ca55
Compare
2ba28e5 to
ad83772
Compare
TCBot Test Analysis
Possible Blockers (0)No blockers found. New Tests (18)
|
9c8c1c1 to
fb5b61e
Compare
TCBot Test Analysis
Possible Blockers (0)No blockers found. New Tests (18)
|
|
JmhPageEvictionBenchmark results on my local machine: master: branch: |
| * while the calling thread already holds entry locks. When set, entries whose locks are contended are skipped | ||
| * (via a non-blocking {@code evictInternal}) instead of blocking, avoiding a lock-ordering deadlock. | ||
| */ | ||
| private static final ThreadLocal<Boolean> EVICT_NON_BLOCKING = new ThreadLocal<>(); |
There was a problem hiding this comment.
It's strange to use thread-local to pass parameter to next stack level. Let's use
evictDataPage(boolean blocking) -> evictDataPage(int pageIdx, boolean blocking)
Instead of
evictDataPageNonBlocking -> set thread-local -> evictDataPage() -> evictDataPage(int pageIdx) -> get thread local.
There was a problem hiding this comment.
Agreed.
Moved the capability into the interface as evictDataPage(boolean tryLock) (default evictDataPage() delegates to false), removed the ThreadLocal.
As a follow-up the method now returns boolean so the size-aware progress guard can count an actual eviction as progress. Parameter named tryLock (matching evictInternal(..., tryLock) / lockEntry(boolean tryLock)) rather than blocking.
| // as a raw IgniteOutOfMemoryException (wrapped into CorruptedFreeListException in the batch path). | ||
| // | ||
| // The re-reserve is an inline demand-eviction: reached from the BPlusTree.invoke row-creation closure, it may | ||
| // re-entrantly remove other entries from the same data tree. That is safe because the closure runs with no |
There was a problem hiding this comment.
That is safe because the closure runs with no data-tree page locks held.
Are you sure about this statement?
As far as I know BPlusTree.invoke holds the write lock on leaf page while execution the closure, so it's not deadlock safe. Deadlock is possible between data-tree leaf pages since page can contain more than one entry (acquired entry lock is not enough protection).
Also, as far as I understand there can be deadlock on expiration: Expiration thread holds write lock on pending tree leaf page and reads data tree (with read lock). While addRow thread holds write lock on data tree leaf page and can concurrently evict data (remove rows) which remove ttl entries and requires write lock on pending tree leaf page.
There was a problem hiding this comment.
Partially correct.
The premise — "BPlusTree.invoke holds the leaf write lock while running the closure" — is not supported by the code: BPlusTree.invokeDown releases the leaf read lock when read(...) returns and takes the leaf write lock only afterwards (in tryInsert/tryReplace/tryRemoveFromLeaf). Your broader point stands: entry tryLock covers only entry-level ordering and does not make eviction page-level lock-free.
The cross-tree (data→pending vs pending→data) residual risk with the TTL worker is real and documented in the code comment; a full fix is out of scope.
Please correct me if I'm wrong.
| */ | ||
| public void addRows(Collection<? extends CacheDataRow> rows, | ||
| IoStatisticsHolder statHolder) throws IgniteCheckedException { | ||
| public void addRows(Collection<? extends CacheDataRow> rows, IoStatisticsHolder statHolder) throws IgniteCheckedException { |
There was a problem hiding this comment.
addRows is only executed on rebalance. Why do we need this new code for rebalance but not for regular put?
reserving only the largest is insufficient
But you do exactly the same. Ensure free space for each row before any row is inserted, so only free space for largest row will be ensured. Other rows reservation will go through writeSinglePage.
There was a problem hiding this comment.
Two points correct, one not.
(1) Rebalance-only: correct — regular puts use addRow (per-put reserve), both paths covered.
(2) Per-row loop == reserve for the largest: correct — a single reserve for the max row is equivalent.
(3) "Other rows go through writeSinglePage" — actually the batch path uses insertDataRows, whose per-page protection now comes from the shared takePageWithReserve re-reserve; the pre-reserve bounds the empty-pages counter up front.
Updated comment.
| } | ||
|
|
||
| if (pageId == 0L) { | ||
| pageId = allocateDataPage(row.partition()); |
There was a problem hiding this comment.
There is also insertDataRows method where page can be requested on rows insert and oom can be fired.
There was a problem hiding this comment.
Valid.
insertDataRows had the same allocateDataPage OOM-firing fallback without the lazy re-reserve, and since IgniteOutOfMemoryException is a RuntimeException, both methods mislabelled it as CorruptedFreeListException.
Made the batch trailing-fragment path symmetric via the shared takePageWithReserve, and both catch clauses rethrow OOM as-is so it is reported as OOM/critical failure, not corruption.
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.